Skip to content

Axis labels should respect the theme (#142) and fix #96 whilst I'm at it. #149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jun 5, 2023
Merged

Axis labels should respect the theme (#142) and fix #96 whilst I'm at it. #149

merged 4 commits into from
Jun 5, 2023

Conversation

samcunliffe
Copy link
Collaborator

@samcunliffe samcunliffe commented Jun 1, 2023 •
edited
Loading

Fixes #142 by reapplying the colour scheme before canvas.draw in NapariMPLWidget._draw (cheers @dstansby!).

Since fixing the colour for axis labels means I need to regenerate the reference scatter plots, I thought I might as well fix #96 by setting the viewer.theme for every test that is marked @pytest.mark.mpl_image_compare. It's not a fix via a nice global stylesheet setting but... you can now read the axes on the baseline figures. (Which is maybe nice??)

Testing:

  • A somewhat trustworthy TDD reproduction is added into test_theme.

Notes for reviewers:

Please double-check for me that a) main really does have #142, and that b) this really does solve it. I have had some strangeness that I am currently blaming on pip install without -e.

@samcunliffe samcunliffe force-pushed the 142-titles-not-respecting-theme branch from a7bdda2 to 03778c7 Compare June 2, 2023 10:33
@samcunliffe samcunliffe force-pushed the 142-titles-not-respecting-theme branch from 359a9fe to 2413b53 Compare June 2, 2023 11:21
@samcunliffe samcunliffe changed the title Failing test that demonstrates #142 Axis labels should respect the theme (#142) and fix #96 whilst I'm at it. Jun 2, 2023
@samcunliffe samcunliffe requested review from dstansby and ruaridhg June 2, 2023 11:31
@samcunliffe samcunliffe added the Bug Something isn't working label Jun 2, 2023
This was linked to issues Jun 2, 2023
Axes text to show up in the test figure #96
Closed
Axis titles are always black. #142
Closed
@samcunliffe samcunliffe marked this pull request as ready for review June 2, 2023 11:33
ruaridhg
ruaridhg approved these changes Jun 2, 2023
View reviewed changes
Copy link
Collaborator

@ruaridhg ruaridhg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The axis labels were black when running napari from main, but running from this branch meant that they were white in dark mode and black in light mode so yay!

@ruaridhg ruaridhg added this pull request to the merge queue Jun 2, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jun 2, 2023
@samcunliffe samcunliffe force-pushed the 142-titles-not-respecting-theme branch from 2413b53 to 4af1c3f Compare June 2, 2023 15:08
@ruaridhg
Copy link
Collaborator

ruaridhg commented Jun 2, 2023 •
edited
Loading

Works locally, but tests are failing on tox because the existing baseline images are using the default theme so adding the them causes a difference. Can figure out why next week.
image

@samcunliffe
Resolves matplotlib#96.
21b5823 
Not a nice global fix, but by setting the test fixture viewer's theme to
light, napari-matplotlib will give axes dark grey axes, labels, and ticks.
@samcunliffe samcunliffe force-pushed the 142-titles-not-respecting-theme branch from 4af1c3f to 21b5823 Compare June 4, 2023 20:31
@samcunliffe
Copy link
Collaborator Author

Works locally, but tests are failing on tox because the existing baseline images are using the default theme so adding the them causes a difference. Can figure out why next week.

I was replacing the plots in tests/baseline but didn't see there are also some baseline plots in tests/scatter/baseline.

Do I need to get into the habit of running pytest with --mpl-generate-path pointing to two directories (I guess with a -k to separate the plots into the scatter subdir?

@samcunliffe samcunliffe requested a review from ruaridhg June 4, 2023 20:36
@ruaridhg
Copy link
Collaborator

ruaridhg commented Jun 5, 2023

Works locally, but tests are failing on tox because the existing baseline images are using the default theme so adding the them causes a difference. Can figure out why next week.

I was replacing the plots in tests/baseline but didn't see there are also some baseline plots in tests/scatter/baseline.

Do I need to get into the habit of running pytest with --mpl-generate-path pointing to two directories (I guess with a -k to separate the plots into the scatter subdir?

Ah yes, that was a recent change by David. I've been running pytest with --mpl-generate-path twice with the different directories, but yes pointing to them both in the same command would be more efficient

@ruaridhg ruaridhg enabled auto-merge June 5, 2023 08:09
ruaridhg
ruaridhg approved these changes Jun 5, 2023
View reviewed changes
Copy link
Collaborator

@ruaridhg ruaridhg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ruaridhg ruaridhg added this pull request to the merge queue Jun 5, 2023
Merged via the queue into matplotlib:main with commit 4a9a4da Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruaridhg ruaridhg ruaridhg approved these changes

@dstansby dstansby Awaiting requested review from dstansby

Assignees
No one assigned
Labels
Bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Axis titles are always black. Axes text to show up in the test figure
3 participants
@samcunliffe @ruaridhg @dstansby